Skip to content

chore: Removed manipulation for custom traceparent header#37121

Merged
nidhi-nair merged 6 commits intoreleasefrom
fix/traceparent-propagation-server
Oct 30, 2024
Merged

chore: Removed manipulation for custom traceparent header#37121
nidhi-nair merged 6 commits intoreleasefrom
fix/traceparent-propagation-server

Conversation

@nidhi-nair
Copy link
Contributor

@nidhi-nair nidhi-nair commented Oct 29, 2024

Description

All Appsmith systems should always rely on a single traceparent header to propagate traces, removing incorrect manipulation to bring back consistency.

Automation

/ok-to-test tags=""

🔍 Cypress test results

Warning

Tests have not run on the HEAD 3aeba4a yet


Wed, 30 Oct 2024 03:57:35 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new observability properties for enhanced metrics tracking, including support for New Relic.
    • Added a new constant for trace management to improve logging capabilities.
  • Bug Fixes

    • Removed outdated web filter to streamline HTTP request tracing.
  • Configuration Changes

    • Updated service names in observability configurations for clarity.
    • Simplified tracing configuration to include all spans without filtering.

These updates enhance the application's observability, allowing for better tracking and monitoring of performance metrics.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2024

Walkthrough

The changes in this pull request involve the removal of the MicrometerTraceHeaderWebFilter class, which managed HTTP tracing headers, and the addition of a new constant TRACE_PARENT in MDCConstants. The RTSCaller class is modified to integrate ObservationRegistry, while the TracingConfig file simplifies span filtering. Configuration properties related to observability have been renamed and updated across several classes, including CommonConfig and MetricsConfig. Additionally, the application.properties file has been updated to reflect new observability settings.

Changes

File Change Summary
.../MicrometerTraceHeaderWebFilter.java Class removed. Managed traceparent and traceparent-otlp headers for HTTP requests.
.../MDCConstants.java Constant added: public static final String TRACE_PARENT = "traceparent";
.../RTSCaller.java Constructor updated to accept ObservationRegistry. Field added: private final ObservationRegistry observationRegistry; WebClient configuration modified.
.../TracingConfig.java Method onlyAppsmithSpans simplified to always return true. Removed specific span filtering logic.
.../instrumentation.ts Updated ATTR_SERVICE_NAME from "rts" to "appsmith-rts".
.../index.ts Modified serviceName in getAppsmithConfigs from "frontend" to "appsmith-client".
.../ConditionalOnMicrometerMetricsEnabled.java Annotation updated to check ${appsmith.observability.metrics.enabled} instead of ${appsmith.micrometer.metrics.enabled}.
.../CommonConfig.java Configuration properties updated to use "observability" namespace instead of "micrometer" for tracing and metrics properties.
.../MetricsConfig.java Constants updated: removed CONTAINER_NAME_KEY, added DEPLOYMENT_NAME_KEY, SERVICE_INSTANCE_ID_KEY, and SERVICE_NAME. Properties related to New Relic removed.
.../application.properties Added new properties for observability and metrics; removed several old properties related to New Relic and Micrometer; modified spring.application.name.

Possibly related PRs

Suggested labels

Task, Bug, ok-to-test, Integrations Product, Integrations Pod General

Suggested reviewers

  • sharat87
  • abhvsn

🎉 In the code where traces did dwell,
A filter was removed, but all is well.
New constants and configs, oh what a sight,
Observability shines, making metrics bright!
With each line refined, the code's now a gem,
Cheers to the changes, let’s celebrate them! 🎊


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label Oct 29, 2024
@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11568406165.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37121.
recreate: .

@github-actions
Copy link

Deploy-Preview-URL: https://ce-37121.dp.appsmith.com

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11568951892.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37121.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a132f6a and 0c7f7fb.

📒 Files selected for processing (1)
  • app/client/src/UITelemetry/auto-otel-web.ts (1 hunks)

[ATTR_DEPLOYMENT_NAME]: deploymentName,
[ATTR_SERVICE_INSTANCE_ID]: serviceInstanceId,
[ATTR_SERVICE_NAME]: serviceName,
"extra": "extra",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Remove the arbitrary "extra": "extra" resource attribute

The "extra": "extra" attribute:

  • Is not used anywhere else in the codebase
  • Doesn't follow the standard resource attributes pattern seen in other OpenTelemetry configurations
  • Adds unnecessary noise to telemetry data
  • Is inconsistent with other Resource configurations in the codebase that only use standard ATTR_ constants
🔗 Analysis chain

Remove arbitrary resource attribute.

The addition of "extra": "extra" seems unnecessary and doesn't follow OpenTelemetry semantic conventions. This attribute will be attached to all telemetry data without providing meaningful context.

- "extra": "extra",

Let's check if this attribute is referenced elsewhere:


Let me verify if this attribute is part of any configuration or has any dependencies.


Let me check one more aspect to see if this is related to any OpenTelemetry configuration or testing setup.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any references to this extra attribute
rg -i '"extra":\s*"extra"' 

Length of output: 94


Script:

#!/bin/bash
# Search for any configuration files or tests that might reference this attribute
rg -l -g '*.{ts,js,json}' -i 'extra' app/client/src/UITelemetry/

# Check if this is part of any interface or type definition
ast-grep --pattern 'interface $_ { $$$ extra: $_; $$$ }'

# Look for any OpenTelemetry configuration patterns
rg -A 5 -B 5 'Resource.default\(\)' app/client/src/UITelemetry/

Length of output: 233


Script:

#!/bin/bash
# Look for the context around this attribute in the file
rg -A 10 -B 10 '"extra":\s*"extra"' app/client/src/UITelemetry/auto-otel-web.ts

# Check for any test files that might be using this
fd -e test.ts -e spec.ts . app/client/src/UITelemetry/ --exec rg -l -i 'extra'

# Look for any resource attribute configurations
ast-grep --pattern 'new Resource({ $$$ })'

Length of output: 2208

@github-actions
Copy link

Deploy-Preview-URL: https://ce-37121.dp.appsmith.com

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11570330015.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37121.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 0c7f7fb and 62fb259.

📒 Files selected for processing (2)
  • app/server/appsmith-interfaces/src/main/java/com/appsmith/external/constants/MDCConstants.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (2 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (2)

20-21: LGTM: Clean import additions for tracing constants


76-79: Verify trace header format compliance

The implementation looks clean, but we should ensure the TRACE_ID value follows the W3C trace context format before setting it as TRACE_PARENT.

✅ Verification successful

Let me gather more context about the trace ID generation and validation.


Let me check one more aspect about the trace context generation.


Trace header implementation is properly validated

The TRACE_ID validation is handled by the Micrometer tracing framework through TraceContext builder in ObservationHelperImpl. The framework ensures W3C trace context compliance, making additional validation in RTSCaller unnecessary.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for TRACE_ID format validation or generation in the codebase
rg -g '!*.{log,lock}' -A 3 "TRACE_ID.*format|TRACE_ID.*generate|TraceContext"

Length of output: 1208


Script:

#!/bin/bash
# Look for TRACE_ID usage and generation in ObservationHelperImpl
ast-grep --pattern 'class ObservationHelperImpl {
  $$$
  $_($$) {
    $$$
    TRACE_ID
    $$$
  }
  $$$
}'

# Check for any trace ID format constants or validation
rg -g '*.java' -B 2 -A 2 'TRACE_ID.*=|TRACE_ID.*pattern|W3C.*trace'

Length of output: 941


Script:

#!/bin/bash
# Look for the traceContextBuilder implementation and usage
rg -g '*.java' -B 3 -A 3 'traceContextBuilder|TraceContext.*build'

# Check if there's any validation in RTSCaller
ast-grep --pattern 'class RTSCaller {
  $$$
  makeRequest($$$) {
    $$$
  }
}'

Length of output: 1183

public static final String THREAD = "thread";
public static final String OBSERVATION = "micrometer.observation";
public static final String TRACE_ID = "traceId";
public static final String TRACE_PARENT = "traceparent";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Potential issue with trace context propagation in RTSCaller

The TRACE_PARENT constant is being used incorrectly in RTSCaller.java. It sets the traceparent header value to just the trace ID, but according to W3C Trace Context spec, the traceparent header should follow the format: version-trace_id-parent_id-trace_flags.

  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java: The header value should be properly formatted instead of just using contextMap.get(TRACE_ID)
🔗 Analysis chain

LGTM! The constant follows W3C trace context specification.

The addition of TRACE_PARENT constant aligns with the standardization of trace propagation headers.

Let's verify the usage of this constant across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of TRACE_PARENT constant and any remaining custom traceparent header manipulations

# Test 1: Find all references to the new constant
echo "=== References to TRACE_PARENT constant ==="
rg "TRACE_PARENT" -A 2

# Test 2: Check for any remaining custom traceparent header manipulations
echo -e "\n=== Check for custom traceparent manipulations ==="
rg -i "traceparent.*header" -g '!*.{md,txt}'

Length of output: 1508


Script:

#!/bin/bash
# Let's verify if there are any other occurrences of traceparent being used directly as a string
# or any other header manipulations related to trace context

# Check for string literal "traceparent" usage
echo "=== Direct traceparent string usage ==="
rg -i '"traceparent"' --type java --type typescript --type javascript -A 2

# Check for trace context related header manipulations
echo -e "\n=== Trace context header manipulations ==="
rg -i '(trace|span).*header' --type java --type typescript --type javascript -A 2

# Check RTSCaller.java implementation to understand the trace context usage
echo -e "\n=== RTSCaller.java implementation ==="
cat app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java

Length of output: 4799

@github-actions
Copy link

Deploy-Preview-URL: https://ce-37121.dp.appsmith.com

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11572125686.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37121.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (1)

37-39: Consider adding null check for observationRegistry.

While Spring typically ensures non-null dependencies, adding a null check would improve robustness.

 public RTSCaller(ObservationRegistry observationRegistry) {
+    if (observationRegistry == null) {
+        throw new IllegalArgumentException("observationRegistry cannot be null");
+    }
     this.observationRegistry = observationRegistry;
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 62fb259 and 4b4b6f2.

📒 Files selected for processing (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (4 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (3)

3-3: LGTM: Clean addition of observation registry.

The import and field declaration follow Java best practices.

Also applies to: 28-28


63-63: LGTM: Proper integration of observation registry.

The WebClient builder is correctly configured with the observation registry.


Line range hint 1-108: Verify complete removal of custom traceparent handling.

Let's ensure no other custom traceparent header manipulations exist in the codebase.

✅ Verification successful

Let me check if this constant is still being used in the codebase.


Custom traceparent handling has been properly removed

The only remaining references to traceparent are:

  • A commented-out block in RTSCaller.java
  • An unused constant definition in MDCConstants.java

Since the code is already commented out and the constant is not being used anywhere else in the codebase, the removal of custom traceparent handling is complete. The new ObservationRegistry integration will handle the tracing properly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining traceparent header manipulations
rg -i "traceparent" --type java

Length of output: 187


Script:

#!/bin/bash
# Search for usages of TRACE_PARENT constant
rg -A 3 "TRACE_PARENT" --type java

Length of output: 1127

Comment on lines +82 to +84
// if (contextMap.containsKey(TRACE_PARENT)) {
// spec.header(TRACE_PARENT, contextMap.get(TRACE_ID));
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove commented out code.

Since the PR's objective is to remove custom traceparent header manipulation, this commented code should be deleted rather than left in place.

-                //                if (contextMap.containsKey(TRACE_PARENT)) {
-                //                    spec.header(TRACE_PARENT, contextMap.get(TRACE_ID));
-                //                }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// if (contextMap.containsKey(TRACE_PARENT)) {
// spec.header(TRACE_PARENT, contextMap.get(TRACE_ID));
// }

@github-actions
Copy link

Deploy-Preview-URL: https://ce-37121.dp.appsmith.com

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11572732420.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37121.
recreate: .

@github-actions
Copy link

Deploy-Preview-URL: https://ce-37121.dp.appsmith.com

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1)

44-54: Update class documentation to reflect new behavior.

The class comment still mentions "filter just Appsmith specific spans" which is no longer accurate.

-/**
- * This configuration file creates beans that are required to filter just Appsmith specific spans
- */
+/**
+ * This configuration file creates beans for managing trace span export and server observations.
+ */
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4b4b6f2 and 134dcf1.

📒 Files selected for processing (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (1)

44-54: Verify impact on trace volume and downstream systems.

The removal of span filtering will increase the volume of trace data being exported. This could affect performance and costs of your observability backend.

Consider implementing sampling if trace volume becomes a concern.

✅ Verification successful

Let me gather more information about the tracing configuration to better understand the impact.


Let me check the OpenTelemetry configuration to understand the sampling setup.


Span filtering removal is safe to proceed

The change to return all spans is aligned with standard OpenTelemetry practices. The system uses OpenTelemetry SDK with configurable sampling through environment variables, so trace volume can be controlled at deployment time without code changes. Previous custom filtering was likely redundant with modern observability platforms' capabilities.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any span volume configurations or sampling rates in the codebase
rg -i "sampling.*rate|span.*filter" --type java

Length of output: 449


Script:

#!/bin/bash
# Check for tracing configuration and sampling related code
rg -i "tracingconfig|samplerconfig|opentelemetry.*sampler" --type java

# Look for any other span filtering or processing logic
ast-grep --pattern 'class $_ implements SpanProcessor { $$$  }'

# Check for any trace exporter configurations
rg -i "trace.*export|opentelemetry.*export" --type java

Length of output: 1627


Script:

#!/bin/bash
# Check OpenTelemetry configuration and initialization
rg -i "opentelemetry.*sdk|tracerprovider|sdktraceprovider" --type java -A 5

# Look for any application properties related to tracing
fd -e properties -e yaml | xargs rg -i "trace|opentelemetry|otel"

Length of output: 3542

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11586709897.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37121.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1)

67-73: Consider documentation updates for observability changes.

Since this is part of a larger change to streamline tracing and observability:

  1. Update configuration documentation to reflect the new property structure
  2. Consider adding migration notes if other services depend on these properties
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java (1)

72-77: LGTM! OpenTelemetry resource attributes properly configured.

The resource attributes are correctly set following OpenTelemetry SDK best practices. Consider adding documentation about these observability changes in your monitoring/observability guide.

Consider adding a code comment explaining the significance of these resource attributes for observability.

app/server/appsmith-server/src/main/resources/application.properties (1)

100-101: Remove temporary configuration comment.

The comment suggests this is a temporary configuration. Consider either removing the comment and property or documenting when this will be deprecated.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 134dcf1 and 3aeba4a.

📒 Files selected for processing (8)
  • app/client/packages/rts/src/instrumentation.ts (1 hunks)
  • app/client/src/ce/configs/index.ts (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java (0 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java (3 hunks)
  • app/server/appsmith-server/src/main/resources/application.properties (2 hunks)
💤 Files with no reviewable changes (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/TracingConfig.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RTSCaller.java
🔇 Additional comments (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/annotations/ConditionalOnMicrometerMetricsEnabled.java (1)

18-18: LGTM! Verify property configuration.

The property path update aligns with the broader observability configuration changes.

Let's verify the property exists in application properties:

✅ Verification successful

Property configuration verified and correctly mapped

The new property path appsmith.observability.metrics.enabled is properly defined in application.properties and mapped to the environment variable APPSMITH_MICROMETER_METRICS_ENABLED with a default value of false.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the new property exists in application properties files
# Expect: Property should be defined in at least one properties file

rg "appsmith\.observability\.metrics\.enabled" -g "*.properties"

Length of output: 219

app/client/packages/rts/src/instrumentation.ts (2)

19-19: LGTM: Service name change aligns with naming convention.

The change from "rts" to "appsmith-rts" provides better service identification in distributed traces.


19-19: Verify trace aggregation in New Relic.

Ensure that existing trace queries and dashboards are updated to use the new service name "appsmith-rts".

app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1)

67-73: LGTM! Verify configuration updates.

The property namespace changes from micrometer to observability align well with the PR objectives. The default values are preserved, maintaining backward compatibility.

Let's verify the configuration updates:

✅ Verification successful

Property namespace changes are properly reflected in application.properties

The verification confirms that:

  • All new observability namespace properties are correctly defined in application.properties
  • No legacy micrometer namespace properties remain in the properties files
  • Environment variable mappings are preserved
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the new property names are defined in application properties
# and if there are any remaining old property references

echo "Checking for new property definitions..."
rg "appsmith\.observability\.(metrics|tracing)" -g "*.properties"

echo "Checking for any remaining old property references..."
rg "appsmith\.micrometer\.(metrics|tracing)" -g "*.properties"

Length of output: 961

app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/MetricsConfig.java (2)

36-39: LGTM! Constants follow OpenTelemetry semantic conventions.

The new constants align with OpenTelemetry's semantic conventions for service and deployment identification.


46-50: Verify application.properties configuration.

The new properties follow Spring's best practices. Please ensure the corresponding properties are defined in application.properties:

  • appsmith.observability.deployment.name
  • appsmith.observability.service.instance.id
✅ Verification successful

Properties are correctly defined in application.properties

Both properties are properly configured with default values:

  • appsmith.observability.deployment.name defaults to "self-hosted"
  • appsmith.observability.service.instance.id defaults to "appsmith-0"
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify property definitions in application.properties
rg "appsmith\.observability\.(deployment\.name|service\.instance\.id)" -g "application*.properties"

Length of output: 384

app/server/appsmith-server/src/main/resources/application.properties (1)

102-107: LGTM! Well-structured observability configuration.

The new observability configuration follows good practices with:

  • Consistent naming convention
  • Appropriate default values
  • Environment variable overrides
app/client/src/ce/configs/index.ts (1)

286-286: Verify service name usage across the codebase.

Let's verify how this service name is used in trace propagation across the codebase.

✅ Verification successful

Service name is correctly used in OpenTelemetry configuration

The service name "appsmith-client" is properly used in the client-side OpenTelemetry configuration and is consistent with the tracing setup. The service name is used as a resource attribute following OpenTelemetry semantic conventions (ATTR_SERVICE_NAME).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to the service name in trace/observability related code
rg -l "appsmith-client" | grep -i "trace\|observ\|telemetry"

# Search for other potential service name definitions
rg -l "serviceName.*appsmith.*client" 

Length of output: 135


Script:

#!/bin/bash
# Let's search for any trace/telemetry configuration or initialization code
rg -A 5 "initializeSentryTracing|initializeTracing|initializeTelemetry" 

# Search for any trace/span related code that might use service names
rg -A 3 "createSpan|startSpan|tracer\.start" 

# Look for any OpenTelemetry related imports and configurations
rg -A 2 "@opentelemetry|opentelemetry"

Length of output: 48755

deploymentName: observabilityDeploymentName.value,
serviceInstanceId: observabilityServiceInstanceId.value,
serviceName: "frontend",
serviceName: "appsmith-client",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider making the service name configurable.

The hardcoded service name "appsmith-client" is added to observability configuration. While this aligns with standardizing trace propagation, consider making it configurable via environment variables for flexibility in different deployment scenarios.

Apply this diff to make the service name configurable:

-      serviceName: "appsmith-client",
+      serviceName: ENV_CONFIG.observability?.serviceName || APPSMITH_FEATURE_CONFIGS?.observability?.serviceName || "appsmith-client",

Add the corresponding type definition to the INJECTED_CONFIGS interface:

observability: {
  deploymentName: string;
  serviceInstanceId: string;
  serviceName: string;
}

@github-actions
Copy link

Deploy-Preview-URL: https://ce-37121.dp.appsmith.com

}

@Bean
SpanExportingPredicate onlyAppsmithSpans() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nidhi-nair now this has been removed. we will allow all spans? won't there be too many and cause clutter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see if this is still true post spring upgrade since the DP is not showing as many spans anymore. We can revisit this if we see a spike perhaps? This allows us to fill the gaps in the trace lineage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes makes sense

@nidhi-nair nidhi-nair merged commit e7e3d5e into release Oct 30, 2024
@nidhi-nair nidhi-nair deleted the fix/traceparent-propagation-server branch October 30, 2024 05:57
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Nov 20, 2024
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Adding this label to a PR prevents it from being listed in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants